-
Notifications
You must be signed in to change notification settings - Fork 622
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ft-benchmark] some fixes for benchmark infra #11604
Conversation
Whoops, we actually able to change some hardcoded values in |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #11604 +/- ##
==========================================
- Coverage 71.52% 71.48% -0.05%
==========================================
Files 786 788 +2
Lines 160573 160774 +201
Branches 160573 160774 +201
==========================================
+ Hits 114849 114923 +74
- Misses 40710 40836 +126
- Partials 5014 5015 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
subprocess.run(f"cargo run -p cli -- insert-ft-transfer {fp.name}", | ||
shell=True) | ||
fp.close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should be able to remove flush
and fsync
by moving this close
call after json.dump
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately no. Probably setting some options while creating tmp file object can help, but I spent a lot of time on this function and didn't find such options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error message was a bit unobvious, so I spent a time playing with tmp file options and place of fp.close()
, before figured out, that it just don't flushing for some reason
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide steps to reproduce this? According to Python documentation, close
is guaranteed to call flush
: https://docs.python.org/3/library/io.html#io.IOBase.close
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to disable delete_on_close
though: https://github.com/python/cpython/blob/cde976d85c6632a908dde1ff8695ac5cafd879b6/Lib/tempfile.py#L540
scripts/ft-benchmark-data-sender.py
Outdated
state_size = (int( | ||
subprocess.check_output(["du", "-s", "~/.near/localnet/node0/data"], | ||
stderr=subprocess.PIPE, | ||
shell=True).decode("utf-8").split()[0]) * 1024) | ||
processed_transactions = [] | ||
time_begin = datetime.now() | ||
while True: | ||
print(f"Data sender loop. Time elapsed: {(datetime.now() - time_begin).seconds} seconds") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use a logging
module for this. It will automatically include the time in the log tine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably time from start of this loop is more informative than just time. But in general I added this just to see if this script started
scripts/ft-benchmark-data-sender.py
Outdated
|
||
|
||
# TODO: send signal to this process if ft-benchmark.sh decided to switch neard to another commit. | ||
# add handling of this signal to this script | ||
if __name__ == "__main__": | ||
|
||
parser = argparse.ArgumentParser(description="Process some integers.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make description more descriptive? :)
scripts/run-ft-benchmark.py
Outdated
@@ -52,9 +52,19 @@ def main() -> None: | |||
|
|||
args = parser.parse_args() | |||
|
|||
time_seconds = args.time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can use some library for this? E.g. there is parse_timespan
in Locust: https://github.com/locustio/locust/blob/1fc3143f1f275c6b311c9cecfab151410a76467e/locust/util/timespan.py#L5
4014886
to
f317f2e
Compare
f317f2e
to
b7f1ab2
Compare
With this PR we will be finally able to run benchmark and send data to DB fully automatically